fix(next): don't log a phantom error for redirect()/notFound() signals#438
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thank you for following the naming conventions! 🙏 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesNext.js navigation signal handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/evlog/src/next/handler.ts`:
- Around line 129-139: Update isNextNavigationSignal to separate the
next/navigation import failure from invoking unstable_rethrow, type
unstable_rethrow as returning void, and return false only when the import or
function availability check fails. Invoke unstable_rethrow outside the import
catch so its thrown unwrapped redirect/not-found signal propagates directly
instead of comparing thrown === error.
In `@packages/evlog/test/next/handler.test.ts`:
- Around line 134-188: Add tests alongside the existing redirect() and
notFound() cases for forbidden() and unauthorized() navigation signals, using
digests NEXT_HTTP_ERROR_FALLBACK;403 and NEXT_HTTP_ERROR_FALLBACK;401
respectively. For each handler, assert the original error is rethrown by
identity and that consoleErrorSpy, consoleSpy, and the drain mock are not
called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2aa19683-f75e-4c71-bd4a-55bf5ba5ffaf
📒 Files selected for processing (3)
.changeset/fix-next-navigation-signal-phantom-error.mdpackages/evlog/src/next/handler.tspackages/evlog/test/next/handler.test.ts
Closes #436
withEvlogtreats Next.js control-flow signals thrown byredirect(),notFound(),forbidden(), andunauthorized()as real application errors: the catch block always calledlogger.error()and defaulted the status to 500 (these signals carry their real status inerror.digest, not.status/.statusCode), leaving a phantom ERROR@500 wide event for every redirect/404.Fix
Detect the signal before logging, using Next's own
unstable_rethrow(fromnext/navigation) so it covers current and future signal types without hardcoding digest strings. If recognized, rethrow untouched — no logging, no emitted event. Otherwise, fall through to the existing error-logging path unchanged.Testing
test/next/handler.test.ts:redirect()signal,notFound()signal, and a real error whose message happens to mentionNEXT_REDIRECT(to confirm we only skip logging based ondigest, not message content).pnpm run test),pnpm run lint,pnpm run typecheck, andpnpm run build:packageall pass.pnpm api:snapshot— no public API changes.Summary by CodeRabbit
redirect(),notFound(), and related responses) could be incorrectly recorded as HTTP 500 errors.error.cause, ensuring the underlying signal is rethrown without triggering error logging.